Repository navigation
Conversation
When TupleOptimization splits a tuple local.set into several local.sets, if a set's value contained a get of a prior tuple element, that value would previously have incorrectly been the updated rather than original element. Avoid reading trampled values by copying the original values to scratch locals before starting to emit the sequence of local sets. Fixes #9210.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: (local.set $2 | ||
| ;; CHECK-NEXT: (local.get $4) | ||
| ;; CHECK-NEXT: ) |
There was a problem hiding this comment.
This is adding extra vars even in trivial cases like this one. How about doing something like ChildLocalizer, conceptually, that is, check for interferences?
Seems like it could be a simple scan of the tuple.make inputs to see that copied fields (from the same tuple) are read in order.
There was a problem hiding this comment.
I experimented with emitting scratch locals only as necessary, but I didn't like how complicated it was. Let me see if I can better encapsulate the complexity.
|
@kripken, see the last commit, which adds a utility for determining precisely when scratch locals are necessary. It seems like a good deal of extra complexity, but maybe it's worth it. |
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Nice, I think this is worth the complexity. In some cases large functions have tons of tuples we need to remove, and this will make this pass much faster in that case.
| (local.get $t) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Please add a test similar to this but where there are two tuples t and u, and the flipping is between them, i.e. not on the same tuple. We should use no temp locals in that case.
| const Module& wasm) | ||
| : interfering(operands.size(), false) { | ||
| Index numOperands = operands.size(); | ||
| if (numOperands <= 1) { |
There was a problem hiding this comment.
This can be an assert, as tuples cannot be of size 1?
| effects.localsRead.end()); | ||
| subsequentWrites.insert(effects.localsWritten.begin(), | ||
| effects.localsWritten.end()); | ||
| if (operands[opIndex]->type == Type::unreachable || |
There was a problem hiding this comment.
Why do we need to check unreachable?
There was a problem hiding this comment.
We don't; I removed this.
|
(also it makes the pass output much easier to reason about) |
This branch has not been deployed
2 participants
When TupleOptimization splits a tuple local.set into several local.sets,
if a set's value contained a get of a prior tuple element, that value
would previously have incorrectly been the updated rather than original
element. Avoid reading trampled values by copying the original values to
scratch locals before starting to emit the sequence of local sets. Do so
only when there is interference that would make it necessary.
Fixes #9210.